Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[110] Upgrade Weaver-Webservice-Core (2.1.1-RC8). #111

Merged
merged 18 commits into from
May 3, 2022
Merged

Conversation

kaladay
Copy link
Contributor

@kaladay kaladay commented Apr 27, 2022

  • Upgrade to Java 11.
  • Upgrade all dependencies with vulnerabilities.
  • Upgrade tests to use Junit5.
  • Add docker file.
  • Add comment about why remaining deprecated code is not replaced.

- Upgrade to Java 11.
- Upgrade all dependencies with vulnerabilities.
- Upgrade tests to use Junit5.
Failure message:
```
Error:  Failed to execute goal org.codehaus.mojo:cobertura-maven-plugin:2.7:instrument (default-cli) on project ir-iiif-service: Execution default-cli of goal org.codehaus.mojo:cobertura-maven-plugin:2.7:instrument failed: Plugin org.codehaus.mojo:cobertura-maven-plugin:2.7 or one of its dependencies could not be resolved: Could not find artifact com.sun:tools:jar:0 at specified path /opt/hostedtoolcache/jdk/11.0.15/x64/../lib/tools.jar -> [Help 1]
```

The tests worked previously prior to adding a version number.
Remove the version number and hope it does not fail.
Cobertura doesn't work under JDK11 and is not well maintained anymore.

Update Coveralls to work under JDK11.
This required bringing in javax.xml.bind:jaxb-api within the plugin of coveralls.
@coveralls
Copy link

coveralls commented Apr 27, 2022

Coverage Status

Coverage decreased (-0.08%) to 83.704% when pulling 5f624af on 110-weaver_updates into cc53172 on main.

@kaladay kaladay linked an issue Apr 27, 2022 that may be closed by this pull request
6 tasks
@kaladay kaladay changed the base branch from sprint_upgrade-staging to main April 28, 2022 13:06
It seems that the group id cannot be passed as a parameter because the `COPY --chown=` command does not support that.
Use a relatively high number, such as 3001 for the uid and gid to reduce potential id conflicts with the system loaded by docker.
Use a single variable ARG declaration for each variable and then import them into each stage.
This must be done because ARG is lost between stages and we really should avoid exposing these via ENV.

Use a custom work directory owned by a created user.
Run maven as that user to avoid doing anything as root (security improvement).
Run the service as that user from within that directory.
The jar is moved and renamed for convenience.

Maven (and possibly java) can be picky about arguments, so use the bracket notation for passing arguments to both maven and java.
@kaladay kaladay marked this pull request as ready for review April 28, 2022 20:30
This was being experimented with and was not intended to be committed.
Dockerfile Outdated Show resolved Hide resolved
…e changes.

Move the hardcoded 3001 into USER_ID argument.
Use `apt-get` instead of `apt`, they are apparently different programs.

It turns out the user does have to be recreated across stages.
Docker was subtly failing and prenenting to perform all of the other steps.

Only the following message is evidence of the problem.
```
linux spec user: unable to find user iriifservice: no matching entries in passwd file
```
This is done either adding repackage to the pom.xml or by adding to the command line, like this:
```
  mvn clean package spring-boot:repackage -Pjar -DskipTests=true
```

This is spring and it should be transparent, so add it to the pom.i
Change it to RUN until such time CMD can be figured out.
…start and not build.

This is why nothing happens.
Revert the change and put CMD back.
ghost
ghost previously approved these changes May 2, 2022
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Will give us upgraded dependencies, improved test logging, and more secure docker build. I advocate for this docker approach be our pattern to follow.

@ghost
Copy link

ghost commented May 2, 2022

@jeremythuff it would be nice to have your sign off before we copy this docker build pattern.

jeremythuff
jeremythuff previously approved these changes May 2, 2022
Copy link
Member

@jeremythuff jeremythuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Move `file:` yaml property up to be alphabetical in ordering.

Increase verbosity of logging to `WARN`.
We probably want to see any warnings when running tests as they may matter.
rladdusaw
rladdusaw previously approved these changes May 2, 2022
Copy link
Contributor

@rladdusaw rladdusaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only change I would suggest is to remove the as runtime from line 43 since it's not used.

The `as runtime` is not being used.
@kaladay kaladay dismissed stale reviews from rladdusaw, jeremythuff, and ghost via 84ef278 May 2, 2022 18:06
rladdusaw
rladdusaw previously approved these changes May 2, 2022
ghost
ghost previously approved these changes May 2, 2022
…blems.

Update the SOURCE_DIR as it doesn't need the leading slash.

The copy over build artifact in the maven stage is not used.
Remove it.
@kaladay kaladay dismissed stale reviews from ghost and rladdusaw via e647e96 May 2, 2022 19:06
jeremythuff
jeremythuff previously approved these changes May 2, 2022
ghost
ghost previously approved these changes May 2, 2022
@kaladay kaladay dismissed stale reviews from ghost and jeremythuff via 5f624af May 3, 2022 15:31
@kaladay kaladay merged commit 6cf8255 into main May 3, 2022
@ghost ghost deleted the 110-weaver_updates branch May 3, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Weaver-Webservice-Core (2.1.1-RC10)
4 participants